Skip to content

Conversation

werdeil
Copy link
Contributor

@werdeil werdeil commented Mar 20, 2020

Following remarks made on PR 281, here is a first version of the EnvironmentTable class which allow to use new hooks.

@BeyondEvil tell me if it is what you had in mind.

Copy link
Contributor

@BeyondEvil BeyondEvil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start!

Please see comments. :)

@@ -336,6 +336,38 @@ def _append_video(self, extra, extra_index, test_index):
)
self.additional_html.append(html.div(html_div, class_="video"))

class EnvironmentTable:
def __init__(self, config):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this big init up into smaller functions.

Think builder pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure to understand what to do here, I agree that everything in the init is not good but the function I can think of (generate_header, generate_row, ...) are very small... Do you want something like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm having trouble articulating the vision I have in my head. Give me the weekend to see if I can throw something together that you can refine. :)

@BeyondEvil
Copy link
Contributor

Regarding builder pattern. We can have a default configuration and then users can use hooks to add to that config.

@werdeil
Copy link
Contributor Author

werdeil commented Mar 23, 2020

Hi @BeyondEvil I just pushed something, tell me if it is what you are thinking when you say "builder pattern"

@werdeil werdeil requested a review from BeyondEvil April 6, 2020 14:40
@ssbarnea
Copy link
Member

How about a rebase?

@werdeil werdeil force-pushed the environment_table branch from a064b92 to 8e14277 Compare May 18, 2020 13:02
@werdeil
Copy link
Contributor Author

werdeil commented May 18, 2020

@ssbarnea Rebase done but there are failing tests, I guess they are new, I'll have a look

@ssbarnea ssbarnea added the enhancement This issue/PR relates to a feature request. label Aug 23, 2020
@BeyondEvil
Copy link
Contributor

I'd like to see a revamp of this for v4 if @werdeil is up for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants